Skip to content

Add browserToolEnabled flag to control browser action visibility #1514

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
cannuri wants to merge 1 commit into from
Closed

Add browserToolEnabled flag to control browser action visibility #1514

cannuri wants to merge 1 commit into from

Conversation

@cannuri
Copy link
Contributor

@cannuri cannuri commented Mar 9, 2025 •
edited by ellipsis-dev bot
Loading

Context

the browser_action instructions were included in system prompt even when Browser usage was turned off in the settings. This PR makes sure that they are only included in system prompt if the browser usage is enabled and the model is supporting computer use.

Implementation

I added browserToolEnabled to the system prompt everywhere where supportsComputerUse was checked.

How to Test

Use a mode that supports computer use, turn Browser tool usage off in the settings, preview system prompt => it should not contain browser_tool instructions.

Use a mode that doesn't support computer use, turn Browser tool usage on in the settings, preview system prompt => it should not contain browser_tool instructions.

Use a mode that supports computer use, turn Browser tool usage on in the settings, preview system prompt => it should contain browser_tool instructions.

Important

Add browserToolEnabled flag to control browser action visibility, updating logic and tests to ensure browser actions are included only when both supportsComputerUse and browserToolEnabled are true.

  • Behavior:
    • Adds browserToolEnabled flag to control browser action visibility in system.ts, capabilities.ts, and rules.ts.
    • Browser actions are included only if both supportsComputerUse and browserToolEnabled are true.
  • Tests:
    • Updates system.test.ts and ClineProvider.test.ts to test scenarios with different combinations of supportsComputerUse and browserToolEnabled.
  • Functions:
    • Modifies getCapabilitiesSection(), getRulesSection(), and getToolDescriptionsForMode() to incorporate browserToolEnabled logic.
    • Updates ClineProvider to handle browserToolEnabled state.

This description was created by Ellipsis for 15cb4ff. It will automatically update as commits are pushed.

@cannuri cannuri requested review from cte and mrubens as code owners March 9, 2025 21:25
@changeset-bot
Copy link

changeset-bot bot commented Mar 9, 2025

⚠️ No Changeset found

Latest commit: 15cb4ff

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. enhancement New feature or request labels Mar 9, 2025
This was linked to issues Mar 9, 2025
When Browser Use is disable the system prompt is not excluding system prompt logic related to browser use #1460
Closed
System prompt contains some browser use logic when model does not support browser use #1461
Closed
@hannesrudolph hannesrudolph added the Issue - In Progress Someone is actively working on this. Should link to a PR soon. label Mar 9, 2025
@hannesrudolph hannesrudolph moved this from New to Issue [In Progress] in Roo Code Roadmap Mar 9, 2025
mrubens
mrubens reviewed Mar 10, 2025
View reviewed changes
Copy link
Collaborator

@mrubens mrubens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought I had logic somewhere that only allowed supportsComputerUse to be true if enableBrowserTool was also true. It seemed a little cleaner than passing another variable through everywhere. Seems like it didn’t work as intended though? I wonder if worth a short investigation into why.

@cannuri
Copy link
Contributor Author

cannuri commented Mar 10, 2025

I thought I had logic somewhere that only allowed supportsComputerUse to be true if enableBrowserTool was also true. It seemed a little cleaner than passing another variable through everywhere. Seems like it didn’t work as intended though? I wonder if worth a short investigation into why.

Have I missed that? I cannot find that logic.
I would not suggest to allow supportsComputerUse to be true if browserToolEnabled is true also, because it might be, that it confuses later for another functionality (for example accessing and controlling GUI applications).
Instead we could combine both into a single one called "browserToolAvailable").

@mrubens
Copy link
Collaborator

mrubens commented Mar 10, 2025

I thought I had logic somewhere that only allowed supportsComputerUse to be true if enableBrowserTool was also true. It seemed a little cleaner than passing another variable through everywhere. Seems like it didn’t work as intended though? I wonder if worth a short investigation into why.

Have I missed that? I cannot find that logic. I would not suggest to allow supportsComputerUse to be true if browserToolEnabled is true also, because it might be, that it confuses later for another functionality (for example accessing and controlling GUI applications). Instead we could combine both into a single one called "browserToolAvailable").

This is the line I was thinking of https://github.com/RooVetGit/Roo-Code/blob/224303660bc66d1a89e6918431828671df1b7d6c/src/core/Cline.ts#L1097

@cannuri
Copy link
Contributor Author

cannuri commented Mar 10, 2025

I thought I had logic somewhere that only allowed supportsComputerUse to be true if enableBrowserTool was also true. It seemed a little cleaner than passing another variable through everywhere. Seems like it didn’t work as intended though? I wonder if worth a short investigation into why.

Have I missed that? I cannot find that logic. I would not suggest to allow supportsComputerUse to be true if browserToolEnabled is true also, because it might be, that it confuses later for another functionality (for example accessing and controlling GUI applications). Instead we could combine both into a single one called "browserToolAvailable").

This is the line I was thinking of

https://github.com/RooVetGit/Roo-Code/blob/224303660bc66d1a89e6918431828671df1b7d6c/src/core/Cline.ts#L1097

Damn, you are right. Let me investigate why it didn't work

@cannuri
Copy link
Contributor Author

cannuri commented Mar 10, 2025 •
edited
Loading

I thought I had logic somewhere that only allowed supportsComputerUse to be true if enableBrowserTool was also true. It seemed a little cleaner than passing another variable through everywhere. Seems like it didn’t work as intended though? I wonder if worth a short investigation into why.

Have I missed that? I cannot find that logic. I would not suggest to allow supportsComputerUse to be true if browserToolEnabled is true also, because it might be, that it confuses later for another functionality (for example accessing and controlling GUI applications). Instead we could combine both into a single one called "browserToolAvailable").

This is the line I was thinking of

https://github.com/RooVetGit/Roo-Code/blob/224303660bc66d1a89e6918431828671df1b7d6c/src/core/Cline.ts#L1097

I think this is valid, but the line in https://github.com/RooVetGit/Roo-Code/blob/main/src/core/webview/ClineProvider.ts#L1847 causes the Preview Prompt to include browser_action. @mrubens I don't fully get why this exists actually. Why is it asking for tool use capabilities of openRouter there?

@hannesrudolph hannesrudolph moved this from Issue [In Progress] to PR [Pre Approval Review] in Roo Code Roadmap Mar 10, 2025
@cannuri cannuri mentioned this pull request Mar 11, 2025
Merged
@mrubens
Copy link
Collaborator

mrubens commented Mar 11, 2025

Thanks for the fix in #1556! I can close this now right?

@mrubens mrubens closed this Mar 11, 2025
@github-project-automation github-project-automation bot moved this from PR [Pre Approval Review] to Done in Roo Code Roadmap Mar 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mrubens mrubens mrubens left review comments

@cte cte Awaiting requested review from cte cte is a code owner

Assignees

@cannuri cannuri

Labels

enhancement New feature or request Issue - In Progress Someone is actively working on this. Should link to a PR soon. size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

3 participants

@cannuri @mrubens @hannesrudolph